Skip to content

Report a pty hangup on every path that can observe one - #278

Merged
jserv merged 1 commit into
sysprog21:mainfrom
open-sources-port:pty-hangup-epoll
Aug 11, 2026
Merged

Report a pty hangup on every path that can observe one#278
jserv merged 1 commit into
sysprog21:mainfrom
open-sources-port:pty-hangup-epoll

Conversation

@doanbaotrung

@doanbaotrung doanbaotrung commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Report a pty hangup on every path that can observe one

epoll_wait never reported EPOLLHUP for a pty master whose slaves had all
closed, and slaves obtained through TIOCGPTPEER were never registered, so
the master's hangup accounting missed them. Wire both, and re-verify the
fd generation a registration pinned at ADD/MOD so a close-and-reuse in the
lookup window cannot charge the hangup to an unrelated file.

Drop proc_pty_any_master_hung_up(). It was meant as a cheap gate that let
epoll skip the registration sweep, but it latches: a master that hangs up
while the guest still holds its fd keeps the gate true for the process's
lifetime, and every epoll_pwait then paid for a full FD_TABLE_SIZE sweep.
Track per-instance active and pty-master counts instead, so the scan is
bounded by what the instance actually registered and skipped entirely when
it holds no pty master.

Delete kqueue events whose guest fd is no longer registered. A
level-triggered event for a deregistered fd is re-reported on every call
and translates to zero epoll events, which spins the caller.

Keep an epoll registration alive while another fd still refers to the same
open file description, matching Linux, and identify that description by
ofd_id at the close chokepoint.

Return ready events in preference to EINTR. Linux ep_poll() checks
ep_events_available() and jumps to send_events before it consults
signal_pending, so EINTR is only the answer for a wait that produced
nothing. Discarding a ready fd to report EINTR loses it in practice: kqueue
re-reports it, but the same pending signal is still there, so the caller is
handed EINTR forever and never drains the fd. A terminal with an unhandled
SIGCHLD spun at 100% CPU this way without ever drawing.

Fix #274


Summary by cubic

Make pty hangups visible on every path and across processes. poll, epoll, read, and readv now report hangups reliably; epoll stamps EPOLLHUP immediately and reads return EIO after draining pending data.

  • Bug Fixes
    • Per-pty shared counters via named shm (fresh on new host pty, join on dup/SCM_RIGHTS/fork-restore; fallback to per-process). Release this process’s remaining slaves at teardown; parent credits inherited slaves before siblings run; child adopts inherited slaves on init.
    • Track slaves opened via TIOCGPTPEER (only if the master generation matches) and mirror dup/dup2 of pty slaves so aliases are counted.
    • Pin fd generations across read/poll/epoll/ioctl to avoid close+reuse races; expose fd_current_generation; resolve host fd and generation atomically.
    • Epoll: stamp EPOLLHUP immediately (no waiting), merge with EPOLLIN when data is pending, bound scan using per-instance active and pty-master counts, honor EPOLLONESHOT, verify registration generation to avoid DEL/ADD races, keep registrations alive while another fd refers to the same open file description (track ofd_id), drop kqueue events for deregistered fds, and return ready events in preference to EINTR.
    • read and readv on hung-up masters return EIO after draining queued data.

Written for commit 26de80c. Summary will update on new commits.

Review in cubic

@doanbaotrung
doanbaotrung marked this pull request as draft August 7, 2026 07:35
cubic-dev-ai[bot]

This comment was marked as resolved.

@doanbaotrung
doanbaotrung force-pushed the pty-hangup-epoll branch 3 times, most recently from 9c25676 to f7bc240 Compare August 7, 2026 09:14
@doanbaotrung
doanbaotrung marked this pull request as ready for review August 7, 2026 09:22
cubic-dev-ai[bot]

This comment was marked as resolved.

@doanbaotrung
doanbaotrung force-pushed the pty-hangup-epoll branch 3 times, most recently from 26891a2 to 3e8dfce Compare August 7, 2026 13:41
@doanbaotrung
doanbaotrung marked this pull request as draft August 7, 2026 13:42
@doanbaotrung
doanbaotrung marked this pull request as ready for review August 7, 2026 13:52

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 10 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/runtime/procemu.c">

<violation number="1" location="src/runtime/procemu.c:2462">
P1: A forked child killed by `SIGKILL` can leave the parent master waiting forever for `POLLHUP`: the host closes the child's slave, but no cleanup runs and `shared->slave_count` stays positive. The accounting needs a death-tolerant reconciliation mechanism instead of relying solely on process-exit cleanup and pty-path reuse.</violation>
</file>

<file name="src/runtime/forkipc.c">

<violation number="1" location="src/runtime/forkipc.c:1964">
P2: A forked child that dies before it runs proc_pty_adopt_inherited_slaves() leaks the parent's per-slave credit in the shared segment, so that pty's master stops reporting hangups permanently. The parent adds one to shared->slave_count per inherited slave in proc_pty_fork_parent_note_inherited() right after it commits the child, and that credit is only returned when the child's local count (set by adopt) is subtracted in proc_pty_release_process_slaves(). The one bail path that runs before adopt -- the fork_ipc_recv_pty_keepalives failure at the top of fork_child_main -- both skips the release call and would not repay the credit even if it called it, because the local count is still 0. The other four bail paths added in this diff all call proc_pty_release_process_slaves(), so this path is an inconsistency and a real leak. Suggest having the child also give back the parent's pre-credit when it bails before adopt (or deferring the parent's credit until the child has adopted), so an aborted fork cannot wedge a master's hangup indefinitely.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread src/syscall/fs.c
Comment thread src/runtime/forkipc.c
Comment thread src/runtime/procemu.c
pty_guest_slave_record_locked(dst_slave_host_fd, pts_num, true);
}

void proc_pty_release_process_slaves(void)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: A forked child killed by SIGKILL can leave the parent master waiting forever for POLLHUP: the host closes the child's slave, but no cleanup runs and shared->slave_count stays positive. The accounting needs a death-tolerant reconciliation mechanism instead of relying solely on process-exit cleanup and pty-path reuse.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/runtime/procemu.c, line 2462:

<comment>A forked child killed by `SIGKILL` can leave the parent master waiting forever for `POLLHUP`: the host closes the child's slave, but no cleanup runs and `shared->slave_count` stays positive. The accounting needs a death-tolerant reconciliation mechanism instead of relying solely on process-exit cleanup and pty-path reuse.</comment>

<file context>
@@ -2078,25 +2388,184 @@ void proc_pty_note_guest_slave(int slave_host_fd, uint32_t linux_pts_num)
+    pty_guest_slave_record_locked(dst_slave_host_fd, pts_num, true);
+}
+
+void proc_pty_release_process_slaves(void)
+{
+    /* Hand back every slave this process still holds, at process teardown.
</file context>

Comment thread src/runtime/procemu.c
Comment thread src/syscall/io.c Outdated
Comment thread src/runtime/forkipc.c
* would stop reporting hangups for good. Still before siblings resume, so
* no guest code can close a slave in between.
*/
proc_pty_fork_parent_note_inherited();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: A forked child that dies before it runs proc_pty_adopt_inherited_slaves() leaks the parent's per-slave credit in the shared segment, so that pty's master stops reporting hangups permanently. The parent adds one to shared->slave_count per inherited slave in proc_pty_fork_parent_note_inherited() right after it commits the child, and that credit is only returned when the child's local count (set by adopt) is subtracted in proc_pty_release_process_slaves(). The one bail path that runs before adopt -- the fork_ipc_recv_pty_keepalives failure at the top of fork_child_main -- both skips the release call and would not repay the credit even if it called it, because the local count is still 0. The other four bail paths added in this diff all call proc_pty_release_process_slaves(), so this path is an inconsistency and a real leak. Suggest having the child also give back the parent's pre-credit when it bails before adopt (or deferring the parent's credit until the child has adopted), so an aborted fork cannot wedge a master's hangup indefinitely.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/runtime/forkipc.c, line 1964:

<comment>A forked child that dies before it runs proc_pty_adopt_inherited_slaves() leaks the parent's per-slave credit in the shared segment, so that pty's master stops reporting hangups permanently. The parent adds one to shared->slave_count per inherited slave in proc_pty_fork_parent_note_inherited() right after it commits the child, and that credit is only returned when the child's local count (set by adopt) is subtracted in proc_pty_release_process_slaves(). The one bail path that runs before adopt -- the fork_ipc_recv_pty_keepalives failure at the top of fork_child_main -- both skips the release call and would not repay the credit even if it called it, because the local count is still 0. The other four bail paths added in this diff all call proc_pty_release_process_slaves(), so this path is an inconsistency and a real leak. Suggest having the child also give back the parent's pre-credit when it bails before adopt (or deferring the parent's credit until the child has adopted), so an aborted fork cannot wedge a master's hangup indefinitely.</comment>

<file context>
@@ -1911,6 +1951,18 @@ int64_t sys_clone(hv_vcpu_t vcpu,
+     * would stop reporting hangups for good. Still before siblings resume, so
+     * no guest code can close a slave in between.
+     */
+    proc_pty_fork_parent_note_inherited();
+
     /* The process-state payload includes the SCM_RIGHTS handoff for region
</file context>

Comment thread src/runtime/procemu.h Outdated
@doanbaotrung
doanbaotrung force-pushed the pty-hangup-epoll branch 2 times, most recently from 142ce7f to aaea84c Compare August 7, 2026 14:56
@jserv
jserv requested a review from Max042004 August 7, 2026 15:15
@doanbaotrung doanbaotrung changed the title Share pty slave accounting across the fork family Report a pty hangup on every path that can observe one Aug 8, 2026

@jserv jserv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase latest main branch and resolve conflicts.

epoll_wait never reported EPOLLHUP for a pty master whose slaves had all
closed, and slaves obtained through TIOCGPTPEER were never registered, so
the master's hangup accounting missed them. Wire both, and re-verify the
fd generation a registration pinned at ADD/MOD so a close-and-reuse in the
lookup window cannot charge the hangup to an unrelated file.

Drop proc_pty_any_master_hung_up(). It was meant as a cheap gate that let
epoll skip the registration sweep, but it latches: a master that hangs up
while the guest still holds its fd keeps the gate true for the process's
lifetime, and every epoll_pwait then paid for a full FD_TABLE_SIZE sweep.
Track per-instance active and pty-master counts instead, so the scan is
bounded by what the instance actually registered and skipped entirely when
it holds no pty master.

Delete kqueue events whose guest fd is no longer registered. A
level-triggered event for a deregistered fd is re-reported on every call
and translates to zero epoll events, which spins the caller.

Keep an epoll registration alive while another fd still refers to the same
open file description, matching Linux, and identify that description by
ofd_id at the close chokepoint.

Return ready events in preference to EINTR. Linux ep_poll() checks
ep_events_available() and jumps to send_events before it consults
signal_pending, so EINTR is only the answer for a wait that produced
nothing. Discarding a ready fd to report EINTR loses it in practice: kqueue
re-reports it, but the same pending signal is still there, so the caller is
handed EINTR forever and never drains the fd. A terminal with an unhandled
SIGCHLD spun at 100% CPU this way without ever drawing.

Fix sysprog21#274
@doanbaotrung

Copy link
Copy Markdown
Collaborator Author

Updated

@jserv
jserv merged commit f86881a into sysprog21:main Aug 11, 2026
21 checks passed
@doanbaotrung
doanbaotrung deleted the pty-hangup-epoll branch August 11, 2026 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pty master hangup is not reported on every path

2 participants